Thanks very much for this (at least potentially!) substantial contribution :)

I won't be reviewing this over the next few days (not stopping anyone else ofc!) but it will definitely be a priority shortly.

The general idea looks right; and to the extent that there are going to be any problems with this, I suspect it's more to do with underlying issues in the tumbler code/processing (I'm particularly thinking of recent issues that have been seen with the --restart option, but there are others). So we may find ourselves needing to look into that, to make this function correctly.

There is another more general architectural/design/philosophy discussion too: where should the 'business logic' be (to the extent that that phrase makes sense, it has to start in jmclient with Taker and Maker objects (btw don't ask me why we don't have a CJPeer object from which these inherit .. ;) ), and bubble up into clients of that code, which now can be defined as RPC-API clients, as well as say Qt or CLI clients. The idea of schedule which I introduced in 2016/17 when refactoring the codebase - and the idea was that we would ditch distinct tumbler vs sendpayment client-level code, and every run is just "Taker runs this schedule" - was so we could have this layer of "you tell me what coinjoins you want to do and I'll just mindlessly do them", where "you" is the aforementioned client, and that client could be the one doing sophisticated assessments/algorithms, of which tumbler is the only one developed so far. As an example, it could go further and do things like: read the current "state" of what coinjoins exist on the blockchain and reconfigure its parameters to look similar (perhaps a silly idea but it illustrates the point).

Whether a web client calling over the RPC-API should be doing any or all of that "business logic layer" is debatable. For now this is for sure the natural starting point. I just hope we can make it work!

Thanks @AdamISZ! Take your time and just let us know once you have feedback.

Some background on our reasoning for where to put the "business logic" part: I personally agree that ideally in future iterations we'd move towards an RPC API interface that just accepts a schedule and executes it "mindlessly". We've kept the "generating the schedule/business logic" part on the server side (for now!), mostly to avoid dragging the current schedule syntax too much into the public interface of the RPC API since there were some discussions around it in the past. For now we just tried to replicate the interface of the tumbler.py script more or less.

Anyway, looking forward to hearing your thoughts! I'm open for any suggestions. :)

Anyway, looking forward to hearing your thoughts! I'm open for any suggestions. :)

I have now tested the PR quite a bit and must say: Works great - no major problems.
After adjusting some parameters, I was able to successfully run an entire schedule from start to end on regtest.

One thing I noticed was that the entries in the schedule log file are duplicated (not in jmwalletd log).
The longer the service runs, the more duplications there are. However, it doesn't look like it has any major impact on functionality.

All in all, I'd say: It works really good and there seems to be no situation a user can't recover from.

Generated docs look good to me too:

Forgot to mention: schedule "monitoring" works as expected too, whatever the GET /taker/schedule endpoint returned was always in sync with the .schedule file in the docker container.

Generated docs look good to me too:

Awesome, thanks for testing that as well! 🙏

OK finished the first round. As you can see from the comments, the basic code structure you're using is definitely the right one as far as I'm concerned, at least to the extent we just want to get 'something working', so to speak.

As the flavour of those comments probably communicate, I can see this being something that 'only just works', not because of anything bad in this PR at all, it's just that tumbler is very tricky. Even from the earliest days, this was the 'flagship product' of Joinmarket (so to speak, heh), but at the same time, by far the hardest thing to have work smoothly from start to finish (I vaguely remember Chris posting a poll on reddit, and a majority couldn't get it to work from start to finish without something going wrong .. a lot of what this repo started from was a desire to fix exactly that (see stallMonitor), but even though it got a lot better, it's still hard.

Another thing I didn't mention is --restart on CLI and the equivalent in Qt. That isn't included here, again it's the "MVP" comment (edit: sorry this was mentioned in the description, above, well, anyway, we agree).

I will test this code and see if it generally works - if it does I think we should merge it in next release but you must inform JAM users that is in a very minimal state (but you can also tell them it's not a disaster if it only half completes ... anyway it has to be looked at in that aspect).

(Sorry here I made a mistake, I forgot that stop_taker is introduced here and handles the tumbler_options var correctly, I had confused it with stop_coinjoin). So ignore what I previously wrote here.

Able to complete a run of tumbler on regtest using curl against a running jmwalletd.py, and without any real difficulty - but that's because I know how!

I'll note here the details of how I did that run. Before I start, it's important to make a fair comparison :) When I test tumbler as a script, or in JoinmarketQt on regtest, I always have to, similarly, use slightly artificial options settings, because of the limited environment that I create with ygrunner.py (3 makers, only).

• Start by running test/ygrunner.py with these settings: pytest --btcconf=/path/to/bitcoin.conf --btcpwd=123456abcdef --nirc=2 -s test/ygrunner.py -p no:warnings (using two local miniircd servers. Still needs a bit of rework for the new onion mcs, but this is fine). Gives me access to a taker wallet.
• Use a pre-existing regtest wallet but w/e we can do that in API if we want
• Before starting jmwalletd, have to edit some settings in the tumbler, because it won't work with defaults, in this environment:

-N : (2,0) (only 3 makers available)
--minmakercount : 2 (as per above, only 3 makers) (important gotcha: this can override minimum_makers in the config! probably should not exist!)
-M : 3 (could be 4 as per default but nice to use final mixdepth for destinations, to check)
-c : (3, 0) (didn't have to change, just prefer a fixed # of txs)
-l: 0.2 (wait tens of seconds, not minutes or hours per tx)

As you can see, none of these changes are necessarily needed for a mainnet run. Though the flexibility is greatly desirable. There are several other settings where the default is absolutely fine, even for testing.

• Fund the wallet; send three payments to mixdepth 0 of the regtest wallet, from the taker wallet that was created by ygrunner (this isn't needed and is a weird historical artifact, I won't bore you with details, feel free to just send coins using bitcoin rpc command). Three payments, because that way there is basically zero chance of a PoDLE commit finding failing even once. You could get away with paying only one coin in, if your test is set up such that a transaction will never fail (I sometimes do tests with a probabilistic failure of the makers).
• Then with the above defaults edited in the code, I start python jmwalletd.py --datadir=. (the datadir for the test joinmarket.cfg instead of the real one)
• Unlock that wallet with $curl 'https://localhost:28183/api/v1/wallet/regtest1.jmdat/unlock' -H "Content-Type: application/json" -d '{"password": "hunter2"}' --silent --insecure
• Note the token (or more practically, disable the check_cookie check). Construct a session check: curl 'https://localhost:28183/api/v1/session' --silent --insecure, coinjoin is obviously shown as not running
• Take three destinations from mixdepth 4 of the regtest wallet, dest1, dest2, dest3 then:
• curl 'https://localhost:28183/api/v1/wallet/regtest1.jmdat/taker/schedule' -H 'Authorization: Bearer ...' --silent --insecure -d '{"destination_addresses": ["dest1", "dest2", "dest3"]}'
• Observe the tumbler run in the log. It goes fine for me like that with the makers coming from ygrunner. These settings, it takes about 5 minutes or so.
• When it's finished, rerun session check. coinjoin in process is false, as expected.
• Check balance and utxos with /display and /utxos and see that the coins are in the right addresses, and add up to slightly less than the starting value.

tACK but with the obvious proviso (which is already implicit in the PR description, to be fair) that this is very bare bones and only really the most basic implementation and isn't guaranteed to always work (but I don't see it in any way as dangerous, hence the ACK part).

@theborakompanioni

One thing I noticed was that the entries in the schedule log file are duplicated (not in jmwalletd log).
The longer the service runs, the more duplications there are. However, it doesn't look like it has any major impact on functionality.

I guess you mean ~/.joinmarket/logs/TUMBLE.log right? Yes this is by design. It's append because it's like a transcript of us trying, in some cases several times. We keep showing the updated schedule over and over per transaction (it changes, where entries act as flags of confirmation, etc).
Unless you meant something different.

I guess you mean ~/.joinmarket/logs/TUMBLE.log right? Yes this is by design. [...] Unless you meant something different.

Yes, I mean the TUMBLE.log file – but this duplication of messages does not look intentional. Saw @dnlggr actually addressing it in a commit on another branch.

It does not impair functionality and it seems a fix is in the making, so it should be fine 💪

For completeness, here is an example (after multiple start/stops on regtest):

2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:34,854 Did not complete successfully, shutting down
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 TUMBLE STARTING
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 
2022-04-26 14:42:47,180 With this schedule: 

Thanks a lot for the detailed review @AdamISZ -- It's very valuable feedback.

I feel like we all agree that this is a MVP at best and that it'll need several iterations to become a robust and well usable for the average person. Until then, we'll take care to not oversell it in the UI and make sure the user is aware of that when using it.

you must inform JAM users that is in a very minimal state

ACK ✅

Regarding the tumbler options: I've been working on an update to this endpoint that allows passing all the tumbler options in addition to the destination addresses (with the default options as fallback). This is especially useful for testing without having to manually edit the tumbler code. This update also fixes the log issues that @theborakompanioni was seeing.

Regarding the naming of the endpoint: I agree that it is slightly confusing. As noted above somewhere, in my opinion the endpoint could definitely evolve into something that takes any schedule and executes it in the future. Let us know if you think we should come up with another name for it until then or if this is fine for now. :)

For completeness, here is an example (after multiple start/stops on regtest):

@theborakompanioni Oh, I see; I hadn't seen that behaviour at all before. Could you open an issue for it?

Does anyone see a reason not to merge this now? The only thing I'm a bit uncertain about is the naming of the endpoint.

That relates to something I simply neglected so far: the API "versioning" (called v1 for now) is not really functioning as versioning should, and I'm not sure how we should be doing it. When it comes to JM itself as software, we just leave it as 0.9.6dev before release and then switch it to 0.9.6 for the actual tagged commit for the release. Since we're still changing this API definition, is there something similar we should do? (obviously at the backend level it's "just part of Joinmarket" itself, so I'm thinking more from the API consumer end).

@theborakompanioni Oh, I see; I hadn't seen that behaviour at all before. Could you open an issue for it?

I noticed, this issue is introduced by this PR. I fixed it in 1d5728f. The problem was that I was creating a new log handler for each time the API was called. It should be fixed now.

That relates to something I simply neglected so far: the API "versioning" (called v1 for now) is not really functioning as versioning should, and I'm not sure how we should be doing it.

I'm also unsure about that. One "hack" would be to keep the naming and in the future extend the endpoint in a backwards compatible way to either:

• If you provide only destination addresses (and possibly tumbler options), it'll keep doing what it currently does: Generate a schedule from the (default) tumbler options and execute that schedule;
• If you provide a schedule, it'll execute it directly ignoring any tumbler options.

That would avoid any breaking changes and thus versioning issues. It's a hack though so I'm not sure if I like it. Open for suggestions. We can also rename the endpoint for now if that is deemed a better solution. :)

No, it's fine. If the endpoint name schedule makes sense to you for now, it's fine.

@AdamISZ Update: I added the possibility to (optionally) override tumbler options in a new PR: #1260

For now, this is useful mostly for testing as discussed above.